Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resource API v1.0 #93

Merged
merged 64 commits into from Mar 28, 2018
Merged

Resource API v1.0 #93

merged 64 commits into from Mar 28, 2018

Conversation

DavidS
Copy link
Contributor

@DavidS DavidS commented Mar 8, 2017

Public PR to facilitate conversations.

This contains draft spec language, code examples, and the history how we got here.

There is still much to do before this can be merged.

DavidS added 16 commits March 8, 2017 14:14
This works for puppet resource and puppet apply.
* describe required call sequences
* note the uniqueness requirements of status reporting per resource instance
* use is/should for old and new attribute values
To make the transition easier, this version re-uses existing language as
far as possible.
Returning a hash was intended to protect against duplicate resources, but
it is a hassle to implement, and duplicate resources at this point were
never an issue in the first place.
@lutter
Copy link

lutter commented Mar 16, 2017

Could this be merged with a big 'Draft - under discussion' disclaimer ? (If need be on a separate branch) That way, others can make PR's against the draft to amend it.

Copy link

@igalic igalic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a few comments… i think i'm still overwhelmed with the amount of stuff here…

docs: 'Additional options to pass to apt-key\'s --keyserver-options.',
},
fingerprint: {
type: 'String',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this then be String[40]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. I'll change it to Pattern[/[a-f0-9]{40}/], as this read-only attribute will always have a specific form. Same for the next two.

read_only: true,
},
expiry: {
# TODO: should be DateTime
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

read_only: true,
},
type: {
type: 'String',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enum[rsa, dsa, ecc, ecdsa]

[
"apt_key { #{values[:name].inspect}: ",
] + values.keys.select { |k| k != :name }.collect { |k| " #{k} => #{values[k].inspect}," } + ['}']
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels very clear and logical, but the rest up there just reads like boilerplate…
what am i missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole file is a mixture of example and prototype library code, I've added an explanation at the top:

# This is a experimental hardcoded implementation of what will be come the Resource API's runtime
# environment. This code is used as test-bed to see that the proposal is technically feasible.

the simpleapt.rb is closer to what a real provider would look like.

@DavidS
Copy link
Contributor Author

DavidS commented Mar 20, 2017

@lutter people can always PR against my branch/fork; collaborators should even be able to push to my branch directly

@igalic thanks for your comments! I'll work them in on my next revision - maybe later this week.

DavidS and others added 8 commits April 4, 2017 17:27
* add explanatory text
* tighten up read_only attribute types
The different kinds of attributes are mutually exclusive. Changing this
to a single key makes it easier to read, and reason about.
This will make the first step to a new provider smaller, while still
providing the power and flexibility if required.
@DavidS
Copy link
Contributor Author

DavidS commented Jul 26, 2017

To keep the loop closed, I'm currently working on a prototype implementation of this in https://github.com/DavidS/puppetlabs-apt/tree/resource-api-experiments

It currently only supports listing apt_key resources through puppet resource. More to come.

DavidS and others added 6 commits September 14, 2017 16:23
Logging is the more central concept and requires explanation first.
Commands can then reference the loglevels, without requiring forward references.
This has been addressed by having the provider be a plain class. Developers
can use regular ruby to build that class in any way they like.

```ruby
class Puppet::Provider::AptKey::AptKey
def set(context, changes, noo: false)
Copy link

@kbarber kbarber Oct 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noop

@DavidS DavidS changed the title (DO NOT MERGE) (DRAFT) (WIP) Resource API Resource API v1.0 Feb 23, 2018
@DavidS
Copy link
Contributor Author

DavidS commented Feb 23, 2018

While it's not yet fully implemented, I'd like to call for a final round of comments on the text as it stands.

A 0.9 of the core concepts is available as https://rubygems.org/gems/puppet-resource_api and is already being trialled by us internally.

@DavidS
Copy link
Contributor Author

DavidS commented Feb 23, 2018

PS: see also pdk new provider in the upcoming v1.4 and the pre-releases available on http://nightlies.puppet.com/

@DavidS
Copy link
Contributor Author

DavidS commented Mar 28, 2018

The Resource API v1.0 gem has been released, so this is now a Thing[tm].

@DavidS DavidS merged commit 2572be3 into puppetlabs:master Mar 28, 2018
@DavidS DavidS deleted the resourceapi branch June 10, 2019 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants